Iis2iclx sensor: move odr and range into dts #32168

avisconti · 2021-02-10T10:27:50Z

STM IIS2ICLX inclinometer sensor

Move odr and range properties from Kconfig to DT.
Moreover, a new yaml file has been created common to I2C and SPI bus.

Tested on x_nucleo_iks01a3 shield.

Create a common binding file that will be included by all bindings handled by iis2iclx driver. Signed-off-by: Armando Visconti <[email protected]>

gmarull

LGTM, two minor comments

gmarull · 2021-02-10T10:30:41Z

drivers/sensor/iis2iclx/iis2iclx.c


-	if (iis2iclx_accel_set_fs_raw(dev,
-				     IIS2ICLX_DEFAULT_ACCEL_FULLSCALE) < 0) {
+	LOG_INF("range is %d", fs);


maybe LOG_DBG?

ok, I was infact in doubt. Seems more for debug I agree.
Thanks

gmarull · 2021-02-10T10:31:18Z

drivers/sensor/iis2iclx/iis2iclx.c


-	iis2iclx->accel_freq = iis2iclx_odr_to_freq_val(CONFIG_IIS2ICLX_ACCEL_ODR);
-	if (iis2iclx_accel_set_odr_raw(dev, CONFIG_IIS2ICLX_ACCEL_ODR) < 0) {
+	LOG_INF("odr is %d", odr);


yes, acked! Thx

Converts iis2iclx range options (500mg, 1g, 2g, 3g) from Kconfigs to Device Tree. Signed-off-by: Armando Visconti <[email protected]>

Move iis2iclx odr options from Kconfigs to Device Tree. Signed-off-by: Armando Visconti <[email protected]>

pabigot

Fine as is; noting an unrelated global change that should also be done separately.

pabigot · 2021-02-10T14:47:38Z

dts/bindings/sensor/st,iis2iclx-common.yaml

+        the signal that is presented to the driver.
+
+    int-pin:
+      type: int


It's out of the PR's intended scope, and the same problem occurs elsewhere so won't block on this, but:

In another PR please review all uses of similar properties in ST devicetree bindings and make this and the others you'll find an enum too, like st,iis2dlpc's drdy-int property.

@pabigot
Yes, that's a good point. I'll do it in a separate PR.
Thanks

avisconti · 2021-02-11T09:42:30Z

@pabigot
I saw that iis2iclx was the only sensor other than iis2dlpc to use int-pin property. So I added the commit in this PR itself. The previous commits are unchanged, and this stuff is planned for v2.6.0 , so it is not an issue if it is delayed.

pabigot

I want more eyes on the potential change to the devicetree content before saying how this should be done.

pabigot · 2021-02-11T12:05:00Z

dts/bindings/sensor/st,iis2iclx-common.yaml

We need to be careful about this, as it breaks anybody's out-of-tree devicetree file that uses int-pin for this property.

Probably best to leave it as is, but if it's going to change both should be present, int-pin should be marked deprecated, and the driver should accept both until int-pin is removed.

If we don't have that documented as a policy somewhere, we should. @galak @mbolivar-nordic

I saw in #31965 that properties with name changed was marked deprecated but still present. I can do the same in fact. Thanks for pointing this out.

pabigot · 2021-02-12T13:12:44Z

drivers/sensor/iis2iclx/iis2iclx.c

I think this should be something like

COND_CODE_1(DT_HAS_PROP(DT_INST(inst), drdy_int), (DT_INST_PROP(inst, drdy_int)), (DT_INST_PROP(inst, int_pin)))

otherwise we're still breaking people. But that could be part of the discussion of how to handle devicetree deprecations.

Yes, correct. I did it too quickly. Sorry.

I'm actually planning to go back to original name, even if I think that drdy-int is a better name, but I'll do it next week. In any case I think it would be productive to understand what is the policy in such a case.

That's the fastest path to getting this in; #32225 should result in a documented policy.

That's the fastest path to getting this in; #32225 should result in a documented policy.

Sure, just did it!

Change int-pin property type to enum, as it may only accept 1 or 2 as possible values. Signed-off-by: Armando Visconti <[email protected]>

pabigot

An intermediate commit has a whitespace issue but checkpatch doesn't see it and it gets cleaned up in the next commit, so this seems fine. Thanks for your work on this.

pabigot · 2021-02-15T11:32:03Z

dts/bindings/sensor/st,iis2iclx-common.yaml

+        - 1 # 3g    (0.122 mg/LSB)
+        - 2 # 1g    (0.031 mg/LSB)
+        - 3 # 2g    (0.061 mg/LSB)
+


git commit normally complains about empty last lines, not sure why it didn't here.

Looks like it's corrected in the next commit.

dts/bindings: iis2iclx: create a common st,iis2iclx-common.yaml

f02e710

Create a common binding file that will be included by all bindings handled by iis2iclx driver. Signed-off-by: Armando Visconti <[email protected]>

avisconti added area: Drivers area: Sensors Sensors area: Devicetree labels Feb 10, 2021

avisconti added this to the v2.6.0 milestone Feb 10, 2021

avisconti requested review from MaureenHelm, gmarull, mbolivar-nordic and pabigot February 10, 2021 10:27

avisconti self-assigned this Feb 10, 2021

github-actions bot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label Feb 10, 2021

gmarull approved these changes Feb 10, 2021

View reviewed changes

avisconti added 2 commits February 10, 2021 13:52

drivers/sensor: iis2iclx: Move range Kconfig property into dts

450c61c

Converts iis2iclx range options (500mg, 1g, 2g, 3g) from Kconfigs to Device Tree. Signed-off-by: Armando Visconti <[email protected]>

drivers/sensor: iis2iclx: Move odr Kconfig property into dts

85353d0

Move iis2iclx odr options from Kconfigs to Device Tree. Signed-off-by: Armando Visconti <[email protected]>

avisconti force-pushed the iis2iclx-move-odr-range-into-dts branch from f44a7f7 to 85353d0 Compare February 10, 2021 12:54

pabigot approved these changes Feb 10, 2021

View reviewed changes

MaureenHelm approved these changes Feb 10, 2021

View reviewed changes

avisconti force-pushed the iis2iclx-move-odr-range-into-dts branch from 94698c7 to a868dc0 Compare February 11, 2021 12:01

avisconti requested a review from nashif as a code owner February 11, 2021 12:01

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Feb 11, 2021

pabigot suggested changes Feb 11, 2021

View reviewed changes

avisconti force-pushed the iis2iclx-move-odr-range-into-dts branch from a868dc0 to 975f2c0 Compare February 12, 2021 13:08

pabigot reviewed Feb 12, 2021

View reviewed changes

avisconti requested a review from pabigot February 12, 2021 13:24

pabigot mentioned this pull request Feb 12, 2021

Document stability levels for devicetree bindings #32225

Open

avisconti added the DNM This PR should not be merged (Do Not Merge) label Feb 12, 2021

drivers/sensor: iis2iclx: modify int-pin type in DT

d144f64

Change int-pin property type to enum, as it may only accept 1 or 2 as possible values. Signed-off-by: Armando Visconti <[email protected]>

avisconti force-pushed the iis2iclx-move-odr-range-into-dts branch from 975f2c0 to d144f64 Compare February 15, 2021 10:04

avisconti removed the DNM This PR should not be merged (Do Not Merge) label Feb 15, 2021

pabigot approved these changes Feb 15, 2021

View reviewed changes

nashif merged commit d2c5664 into zephyrproject-rtos:master Feb 15, 2021

avisconti deleted the iis2iclx-move-odr-range-into-dts branch February 15, 2021 16:25

Iis2iclx sensor: move odr and range into dts #32168

Iis2iclx sensor: move odr and range into dts #32168

Uh oh!

Conversation

avisconti commented Feb 10, 2021

Uh oh!

gmarull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pabigot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avisconti commented Feb 11, 2021

Uh oh!

pabigot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avisconti Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pabigot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

avisconti Feb 12, 2021 •

edited

Loading